Database context - Run server level statements on the connection - #10580
Database context - Run server level statements on the connection#10580andreasjordan wants to merge 1 commit into
Conversation
Ten call sites went through a database object only because they needed somewhere to run a statement. The execution manager of an SMO database is the connection context of the parent server, so each of them issued a USE and left the connection of the caller in master or msdb. None of the statements needed a database context in the first place. They now run on the connection itself: - Export-DbaLogin, New-DbaLogin, Get-LoginPasswordHash read a password hash from sys.sql_logins or sys.server_principals. In all three the primary path already used ConnectionContext.ExecuteScalar and only the fallback went through master. - Get-DbaDbDetachedFileInfo resolves a collation with fn_helpcollations, which is available in every database. - Get-OfflineSqlFileStructure reads SERVERPROPERTY. - Set-DbaTempDbConfig executes ALTER DATABASE tempdb statements. - Remove-DbaAgentJob called sp_delete_job in msdb. The procedure is now named in full as msdb.dbo.sp_delete_job, so the connection does not have to go there. The help of Connect-DbaInstance recommended the pattern this removes, so it now points at the connection context and says why. This is the part of #10555 that needs no new mechanism, so it is separate from the script method fix in #10579. Set-DbaTempDbConfig also reads tempdb through $server.Databases['tempdb'].Query(), which is that other fix; the command is only free of the leak once both are in. (do Export-DbaLogin, New-DbaLogin, Get-DbaDbDetachedFileInfo, Set-DbaTempDbConfig, Remove-DbaAgentJob, Sync-DbaLoginPassword, Connect-DbaInstance) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
potatoqualitee
left a comment
There was a problem hiding this comment.
Findings
[P1] Set-DbaTempDbConfig -Force still leaves the caller in tempdb
File: public/Set-DbaTempDbConfig.ps1:364
The forced file-reduction path adds batches beginning with USE [tempdb] to $sql for every file being emptied and removed. The PR then executes that array directly through the caller鈥檚 ConnectionContext. This avoids SMO鈥檚 implicit switch to master, but it does nothing about the explicit USE [tempdb] already inside the SQL. After a successful reduction, or an error after the first such batch, a shared non-pooled connection remains in tempdb.
This also remains broken after PR #10579 because these statements bypass the Query and Invoke wrappers entirely. The existing real-instance -Force test verifies the removed files and preserved data but never checks DB_NAME() afterward.
The execution needs either:
- A
try/finallythat remembers and restoresConnectionContext.CurrentDatabase. - A copied connection targeted to
tempdb, since these operations do not depend on the caller鈥檚 session state.
The regression test should use a fresh -NonPooledConnection beginning in master or msdb, perform an actual forced reduction, and then assert the original database.
[P2] The new collation test passes even when the changed query fails
File: tests/Get-DbaDbDetachedFileInfo.Tests.ps1:94
Get-DbaDbDetachedFileInfo catches every failure from the new ConnectionContext.ExecuteWithResults() call and falls back to the numeric collation ID. The test only asserts that Collation is non-null and nonempty, so it passes even if the changed call always throws and never resolves a collation name.
Capture the expected collation before detaching the database and compare it directly:
$expectedCollation = $server.Databases[$dbname].Collation
# detach and execute command
$callerResult.Collation | Should -Be $expectedCollation[P3] The new help text overgeneralizes the safe pattern
File: public/Connect-DbaInstance.ps1:24
The advice now says to run statements through ConnectionContext rather than a database object. That is correct only for statements that genuinely do not require a database context. A database-scoped statement sent through ConnectionContext executes in whatever database the session currently occupies, which can be just as dangerous.
I would change it to something like:
For server-scoped statements that do not depend on a particular database, execute through the connection context rather than a database object.
Verdict
Request changes. The login/hash, detached-file, offline-file and Agent substitutions otherwise look mechanically correct. The primary Actions run is green, but it does not cover the remaining Set-DbaTempDbConfig -Force context leak.
Type of Change
Purpose
Step two of #10555, and the half that needs no new mechanism, so it is independent of #10579.
Ten call sites went through a database object only because they needed somewhere to run a statement. The execution manager of an SMO database is the connection context of the parent server, so each of them issued a
USEand left the connection of the caller inmasterormsdb. None of the statements needed a database context at all.The clearest example is
Export-DbaLogin.ps1:391-393, where the primary path was already right and only the fallback was not:Approach
Each statement now runs on the connection itself, so there is no context to put back:
Export-DbaLogin,New-DbaLogin,Get-LoginPasswordHashsys.sql_logins/sys.server_principalsGet-DbaDbDetachedFileInfofn_helpcollations()Get-OfflineSqlFileStructureSERVERPROPERTY(...)Set-DbaTempDbConfigALTER DATABASE tempdb ...Remove-DbaAgentJobsp_delete_jobmsdb.dbo.sp_delete_jobOnly the last one changes a statement. Verified that the three part name works and leaves the context alone:
Also verified that
ConnectionContext.ExecuteNonQuerytakes the string arraySet-DbaTempDbConfighands it, which is the one signature difference between the two receivers.The help of
Connect-DbaInstancerecommended the pattern this removes:It now points at the connection context and says why.
Tests
Regression contexts for the two sites that a test can actually reach, both asserting on a
-NonPooledConnection, because SMO reopens a pooled one at its default database and hides the leak:Remove-DbaAgentJobstill removes the job and leaves the connection where it wasGet-DbaDbDetachedFileInfostill reads the file and resolves the collation, and leaves a connection that was onmsdbonmsdbThe other sites have no reachable test, and it is worth being explicit about why rather than adding a test that proves nothing:
catchfallbacks forExecuteScalar, which does not fail on a supported SQL Server, so the changed line does not runGet-OfflineSqlFileStructurehas no callers anywhere in the module; the change is correct but the function is dead codeSet-DbaTempDbConfigreaches its changed line only by really reconfiguring tempdb. The existingSet-DbaTempDbConfigtests do execute that path, so it is exercised, but a context assertion cannot pass yet - see below.Test files run:
Export-DbaLogin(20),New-DbaLogin(15),Get-DbaDbDetachedFileInfo(9),Remove-DbaAgentJob(12 + 1 skipped),Sync-DbaLoginPassword(9), all passing.Set-DbaTempDbConfig is only half fixed by this
It also reads tempdb through
$server.Databases['tempdb'].Query(...)in three places, which is the script method fix in #10579. Measured on this branch, the connection still ends up in tempdb from those calls even when the command stops before the line changed here:So the command is free of the leak only once both changes are in, and a context test for it belongs with the second one.
馃 Generated with Claude Code